Skip to content

Conversation

@ChristopherBiscardi
Copy link
Contributor

@ChristopherBiscardi ChristopherBiscardi commented Dec 25, 2025

Upgrade to wgpu 28

Important

This can't merge until bevyengine/naga_oil#132 does, and the dependency is updated from my fork to the release.

Also requires wgpu 28 in dlss_wgpu: bevyengine/dlss_wgpu#17

Note

This does not enable mesh shaders, and neither does the naga_oil PR. I chose to do an upgrade first, then go back and see about mesh shaders.

Here's a general list of changes and what I did. Commits are grouped by feature except for the last one, which enabled solari when I ran the solari example.

MipmapFilterMode is split from FilterMode

solution: implement From for MipmapFilterMode/ImageFilterMode since the values are the same. The split was because the spec indicates they are different types, even though they have the same values.

Push Constants are now Immediates

immediate_size is a u32 so use that instead of PushConstantRange

Capabilities name changes

Got new list from https://github.com/gfx-rs/wgpu/blob/trunk/wgpu-core/src/device/mod.rs#L449 and copied it in.

subgroup_{min,max}_size moved from Limits to AdapterInfo

Update the limits to have the fields they have now, mirror the logic from the other limits calls.

multiview_mask

set to None because we don't use it currently. Its vaguely for VR.

error scope is now a guard

retain guard and then pop() it later


I made one mistake during the PR, thinking set_immediates was going to be the size of the immediates and not the offset. I'd like reviewers to take a look at immediates offset and size sites specifically just in case I missed something.

Here's a bunch of examples running

screenshot-2025-12-24-at-16 47 22@2x screenshot-2025-12-24-at-16 48 44@2x screenshot-2025-12-24-at-16 49 10@2x screenshot-2025-12-24-at-16 49 31@2x

Known Issues

Note

There are no current known issues. Everything previous has been solved.

  • enable extensions can not be written in naga oil in a way that allows them to be used in composed modules.

Update: this was fixed by introducing a flag in naga-oil to force shaders to allow ray queries in composed modules when using bevy_solari. This is temporary and will be different in WESL-future.

old explanation This is blocking solari from working on nvidia (solari runs successfully on macos m1) because it needs `enable wgpu_ray_query;`. Putting the declaration before `#define_import_path` means it gets stripped out (afaict), and putting it after results in
error: expected global declaration, but found a global directive
  ┌─ embedded://bevy_solari/scene/raytracing_scene_bindings.wgsl:3:1
  │
3 │ enable wgpu_ray_query;
  │ ^^^^^^ written after first global declaration
  │
  = expected global declaration, but found a global directive
Previous notes as dlss_wgpu was being fixed here

The wgpu release notes don't mention which PR this was introduced in, only saying:

Using both the wgpu command encoding APIs and CommandEncoder::as_hal_mut on the same encoder will now result in a panic.

It was caused by gfx-rs/wgpu#8373 which claimed to not know of any use cases

With record on finish, the actual ordering on the command buffer is deeply counter intuitive (all as_hal would come first) and I think it additionally was just flat out broken in some ways

Possible path forward is using multiple command buffers: https://discord.com/channels/691052431525675048/743663924229963868/1453795633503670415

@alice-i-cecile alice-i-cecile added this to the 0.19 milestone Dec 25, 2025
flags: instance_descriptor.flags,
memory_budget_thresholds: instance_descriptor.memory_budget_thresholds,
backend_options: instance_descriptor.backend_options.clone(),
telemetry: None
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

telemetry seems to be very firefox-oriented, so I've set it to None.

from: gfx-rs/wgpu#8576

name.starts_with("Radeon")
|| name.starts_with("AMD")
|| name.starts_with("Intel")
bevy_tasks::IoTaskPool::get().scope(|scope| {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to enumerate_adapters being async now. Commenting because its probably worth a look for reviewers above the mass of other changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, yeah, interesting. We want to revisit this eventually but since it's for Vulkan blocking here should be fine?

@github-actions
Copy link
Contributor

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

@ChristopherBiscardi ChristopherBiscardi added A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 25, 2025
@ChristopherBiscardi ChristopherBiscardi added the C-Dependencies A change to the crates that Bevy depends on label Dec 25, 2025
Copy link
Contributor

@andriyDev andriyDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. AFAICT all the immediates are the correct sizes.

Copy link
Member

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't closely audit the feature/caps changes but assuming you read the changelog correctly but otherwise lgtm pending full regression passes.

name.starts_with("Radeon")
|| name.starts_with("AMD")
|| name.starts_with("Intel")
bevy_tasks::IoTaskPool::get().scope(|scope| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, yeah, interesting. We want to revisit this eventually but since it's for Vulkan blocking here should be fine?

pub push_constant_ranges: Vec<PushConstantRange>,
/// The immediate size for this pipeline.
/// Supply 0 if the pipeline doesn't use immediates.
pub immediate_size: u32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if we might just add a parenthetical for search "(also known as push consants)" or something similar.

let time_span = diagnostics.time_span(command_encoder, "dlss_super_resolution");

dlss_context
let dlss_command_buffer = dlss_context
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? I might be missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixing the wgpu encoding API with the raw encoding API is not longer allowed, so dlss_wgpu needed some changes, and this reflects the new usage.

@ChristopherBiscardi
Copy link
Contributor Author

didn't closely audit the feature/caps changes but assuming you read the changelog correctly

The features/caps changes are an entire file we copied from wgpu, so it should be resistant to small human errors in that way.

tracing = { version = "0.1", default-features = false, features = ["std"] }
dlss_wgpu = { version = "2", optional = true }
# dlss_wgpu = { version = "2", optional = true }
dlss_wgpu = { git = "https://github.com/ChristopherBiscardi/dlss_wgpu.git", branch = "wgpu-28", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to land upstream and have a release before we can merge this PR.

};

render_device
let scope = render_device
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps call this variable error_scope?

Comment on lines -434 to -439
min_subgroup_size: limits
.min_subgroup_size
.max(constrained_limits.min_subgroup_size),
max_subgroup_size: limits
.max_subgroup_size
.min(constrained_limits.max_subgroup_size),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting in-line for other reviewers that these subgroup limits were moved to adapter info.

Comment on lines -29 to +38
naga_oil = { version = "0.20", default-features = false, features = [
# naga_oil = { version = "0.20", default-features = false, features = [
# "test_shader",
# ] }
naga_oil = { git = "https://github.com/ChristopherBiscardi/naga_oil.git", branch = "naga-28", default-features = false, features = [
"test_shader",
] }

[target.'cfg(target_arch = "wasm32")'.dependencies]
naga_oil = { version = "0.20" }
# naga_oil = { version = "0.20" }
naga_oil = { git = "https://github.com/ChristopherBiscardi/naga_oil.git", branch = "naga-28" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting the git repo dependencies here for visibility as something that needs to be fixed before merging this PR.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to approve, but there is one missing wgpu capability (CULL_DISTANCE) and there are git dependencies (naga_oil and dlss_wgpu) that need to be resolved with updates and releases in those crates before merging this.

@ChristopherBiscardi
Copy link
Contributor Author

CULL_DISTANCE is not in Features, so doesn't need to be mapped.

Comment on lines 161 to 162
time_span.end(command_encoder);
command_encoder.pop_debug_group();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to move these to a new command encoder after add_command_buffer.

We basically need 3 command encoders:

  1. Regular bevy encoder, puts start timestamp before dlss, and records the barriers needed before dlss
  2. Dlss encoder
  3. Regular bevy encoder, puts end timestamp after dlss

However I think we'll need some changes to bevy's diagnostics API in order to make this work, as currently it expects you to use the same encoder to start/end a timestamp query iirc.

I can work on this after this PR, but just wanted to note it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

6 participants